Add support for async fixtures #430

olafurpg · 2021-10-10T20:21:44Z

Previously, it was not possible to create fixtures that loaded
asynchronously. It was possible to work around this limitation on the
JVM by awaiting on futures, but there was no workaround for Scala.js.
This commit adds support to return futures (and anything that converts
to futures) from the beforeAll/beforeEach/afterEach/afterAll methods.

Supersedes #418. This commit is
inspired by that PR but uses a different approach:

No new AsyncFixture[T] type. This simplies the logic in
MUnitRunner and reduces the size of the MUnit public API.
The downside is that we introduce a breaking change to the existing
Fixture[T] API, which will require bumping the version to v1.0.0
for the next release.
This approach supports any Future-like values by hooking into the
default evaluation of test bodies via munitValueTransforms.

Co-authored-by: Daniel Esik e.danicheg@yandex.ru

Previously, it was not possible to create fixtures that loaded asynchronously. It was possible to work around this limitation on the JVM by awaiting on futures, but there was no workaround for Scala.js. This commit adds support to return futures (and anything that converts to futures) from the beforeAll/beforeEach/afterEach/afterAll methods. Supersedes scalameta#418. This commit is inspired by that PR but uses a different approach: - No new `AsyncFixture[T]` type. This simplies the logic in `MUnitRunner` and reduces the size of the MUnit public API. The downside is that we introduce a breaking change to the existing `Fixture[T]` API, which will require bumping the version to v1.0.0 for the next release. - This approach supports any `Future`-like values by hooking into the default evaluation of test bodies via `munitValueTransforms`. Co-authored-by: Daniel Esik <e.danicheg@yandex.ru>

olafurpg · 2021-10-10T20:28:28Z

I'll try to get back to this PR soon so that it doesn't stay open for too long.

A bug in Scala 2.11/2.12 made `TimeoutSuite` fail with the new async fixtures. This commit changes how we implement `munitTimeout` to race two futures instead of calling `Await.result`.

This test case was causing CI failures on Linux that I wasn't able to reproduce on my local MacBook.

Previously, the test suite was using a parasitic execution context.

olafurpg · 2021-10-11T11:42:54Z

CI is green now but I haven't properly reviewed the changes yet. There are several moving pieces and still some breaking behavioral changes that need to be documented (for example ordering of fixtures).

olafurpg · 2021-10-12T06:33:08Z

I'd love to get some input on one design decision. We have three options:

Only support Fixture[T] where lifecycle methods return Any
(current PR) Same as option 1 but additionally add UnitFixture[T] and FutureFixture[T] where lifecycle methods return Unit and Future[Unit] respectively.
(my personal preference) Introduce AnyFixture[T] where lifecycle methods return Any, let Fixture[T] methods return Unit and FutureFixture[T] where methods return Future[Unit].

The current PR uses Option 2 but I don't like the name UnitFixture so I'm leaning towards Option 3 because I want to avoid the situation where IDEs autocomplete Any when you implement Fixture[T]. cc/ @gabro

gabro · 2021-10-14T09:49:39Z

Just to throw something out there, have you considered Fixture[T, R] where R is the result type?

olafurpg · 2021-10-14T10:48:31Z

@gabro I considered it but didn't want to write override def munitFixtures: List[Fixture[Id, String]] or override def munitFixtures: List[Fixture[_, _]] for common cases. Under the hood, MUnit works with Any and Future[Any] so it doesn't buy us cleaner internal code. For end users, I feel like Fixture[String] and FutureFixture[String] and TaskFixture[String] will be more ergonomic to write and it will result in more readable code.

olafurpg · 2021-10-14T11:19:10Z

For the record, MUnit has abstracted on the container type Future[_] from the start and it's been possible to write custom Suite classes for other containers like IO. The idea was that munit.FunSuite was special cased for Future[_] but you could implement test suites based on another container. This PR removes that functionality because MUnit internals have always been tied to Future anyways and most users seemed fine with using munit.FunSuite directly (even to integrate with cats-effect IO).

gabro · 2021-10-14T11:42:40Z

Ok, I see it would be a tad annoying to deal with the extra type parameter all of the time.

(my personal preference) Introduce AnyFixture[T] where lifecycle methods return Any, let Fixture[T] methods return Unit and FutureFixture[T] where methods return Future[Unit].

About this, how would one go about implementing IOFixture[T] for instance?

armanbilge · 2021-10-14T11:46:34Z

About this, how would one go about implementing IOFixture[T] for instance?

By converting the IO to a Future (scaladoc).

Edit: here's an example: https://github.com/typelevel/munit-cats-effect/blob/main/common/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

gabro · 2021-10-14T11:54:02Z

So it would extend FutureFixture and apply a transform. Sounds reasonable.

In this case I'm 👍 for your solution number 3, @olafurpg, seems to be the most ergonomic

olafurpg · 2021-10-14T12:32:49Z

It's a bit confusing, but IOFixture would need to extend AnyFixture since the lifecycle method signatures should return IO[Unit]. Value transforms would take care of converting those IO values into futures.

armanbilge · 2021-10-14T12:35:39Z

Well, most likely users won't be expected to work with the lifecycle methods directly since Cats Effect already has Resource exactly for this. So, munit-cats-effect will take a Resource[IO, A] and turn it into a fixture, just like in the example I linked above.

olafurpg · 2021-10-14T12:36:18Z

Edit: here's an example: https://github.com/typelevel/munit-cats-effect/blob/main/common/shared/src/main/scala/munit/CatsEffectFunFixtures.scala

Also confusingly, FunFixture are actually not using Fixture[T]. Fun fixtures are test-local only and require you to re-define the fixture variable name for every test, unlike Fixture[T] where you define the variable once per-suite and reference it via the apply() method.

armanbilge · 2021-10-14T12:38:23Z

Fun fixtures are test-local only and require you to re-define the fixture variable name for every test,

Yes, I'm aware, sorry for the confusion :) I linked that example because it shows the conversion to Future. There's also a fixture implementation that is JVM-only since it relies on a blocking call:

https://github.com/typelevel/munit-cats-effect/blob/main/common/jvm/src/main/scala/munit/CatsEffectFixturesPlatform.scala

olafurpg · 2021-10-14T12:38:47Z

It would be a good exercise to implement the new Fixture API with Resource. I know some people have achieved it on the JVM with blocking I/O and it's not pretty. The code won't be much prettier with this PR since Fixture[_] is inherently a state-ful API, but it will at least avoid the blocking I/O making it possible to use MUni async fixtures on Scala.js.

armanbilge · 2021-10-14T12:39:52Z

It would be a good exercise to implement the new Fixture API with Resource.

If you publish a snapshot of this PR, I'd be happy to make a PR against munit-cats-effect demonstrating what this might look like.

olafurpg · 2021-10-14T12:41:06Z

Sounds good. I'll try to get this PR in shape before the end of this week.

armanbilge · 2021-10-14T12:42:16Z

Thanks! Really excited about this.

Previously, the `Fixture` lifecycle methods returned `Any`, which meant that auto-completions would insert `Any` in the result type when auto-generating implementation of a fixture. Now, the `Fixture[T]` class has `Unit` as result types for consistency with older versions of MUnit. Instead, we introduce `AnyFixture[T]` which only needs to be used when comibing a list of `Fixture[T]` and `FutureFixture[T]` (along with 3rdparty fixture classes).

olafurpg · 2021-10-16T08:13:44Z

I went ahead and implemented option 3 from the alternatives here #430 (comment) This came out much cleaner compared to the previous implementation where it was very easy to auto-complete Any for all life-cycle methods.

olafurpg · 2021-10-16T08:33:57Z

Will go ahead and merge this and cut a milestone release. I have a few more breaking changes that I want to include in the final version of v1.0.0 so please keep that in mind before upgrading everything to the milestone release

olafurpg force-pushed the async-fixtures branch from eaf6fdc to 69175cb Compare October 10, 2021 20:22

olafurpg mentioned this pull request Oct 10, 2021

Add asynchronous fixture #418

Closed

olafurpg added 2 commits October 11, 2021 09:22

Make tests pass

995c22b

A bug in Scala 2.11/2.12 made `TimeoutSuite` fail with the new async fixtures. This commit changes how we implement `munitTimeout` to race two futures instead of calling `Await.result`.

Add MiMa excludes for breaking changes

a9c06df

olafurpg force-pushed the async-fixtures branch 2 times, most recently from a0d3f58 to 65704c9 Compare October 11, 2021 08:33

Cleanup code

cfc64be

olafurpg force-pushed the async-fixtures branch from 65704c9 to cfc64be Compare October 11, 2021 10:22

Optimization: don't run timeout logic for non-async tests

8a123cd

olafurpg force-pushed the async-fixtures branch from d277a57 to 8a123cd Compare October 11, 2021 10:49

Remove infinite loop test for TimeoutSuite

2598fa6

This test case was causing CI failures on Linux that I wasn't able to reproduce on my local MacBook.

olafurpg force-pushed the async-fixtures branch from 5eac47a to 2598fa6 Compare October 11, 2021 10:57

Add infinite loop test back with a new execution context

68f9e6c

Previously, the test suite was using a parasitic execution context.

olafurpg added 2 commits October 16, 2021 09:58

Add more docs for the new fixtures

1b3854e

Fix failing async fixture tests

9e081b3

olafurpg force-pushed the async-fixtures branch from 30c3052 to 9e081b3 Compare October 16, 2021 08:26

olafurpg mentioned this pull request Oct 16, 2021

Move Fixture[T] into a toplevel class. #228

Closed

olafurpg merged commit a2f1b91 into scalameta:main Oct 16, 2021

olafurpg deleted the async-fixtures branch October 16, 2021 08:34

olafurpg mentioned this pull request Oct 16, 2021

When an async test times out, any fixtures used in the tests are not torn down #285

Closed

armanbilge mentioned this pull request Jun 19, 2022

afterAll() does not run after all fixtures have been torn down #539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for async fixtures #430

Add support for async fixtures #430

olafurpg commented Oct 10, 2021

olafurpg commented Oct 10, 2021

olafurpg commented Oct 11, 2021

olafurpg commented Oct 12, 2021

gabro commented Oct 14, 2021

olafurpg commented Oct 14, 2021 •

edited

Loading

olafurpg commented Oct 14, 2021

gabro commented Oct 14, 2021

armanbilge commented Oct 14, 2021 •

edited

Loading

gabro commented Oct 14, 2021 •

edited

Loading

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 14, 2021 •

edited

Loading

armanbilge commented Oct 14, 2021 •

edited

Loading

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 16, 2021

olafurpg commented Oct 16, 2021

Add support for async fixtures #430

Add support for async fixtures #430

Conversation

olafurpg commented Oct 10, 2021

olafurpg commented Oct 10, 2021

olafurpg commented Oct 11, 2021

olafurpg commented Oct 12, 2021

gabro commented Oct 14, 2021

olafurpg commented Oct 14, 2021 • edited Loading

olafurpg commented Oct 14, 2021

gabro commented Oct 14, 2021

armanbilge commented Oct 14, 2021 • edited Loading

gabro commented Oct 14, 2021 • edited Loading

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 14, 2021 • edited Loading

armanbilge commented Oct 14, 2021 • edited Loading

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 14, 2021

armanbilge commented Oct 14, 2021

olafurpg commented Oct 16, 2021

olafurpg commented Oct 16, 2021

olafurpg commented Oct 14, 2021 •

edited

Loading

armanbilge commented Oct 14, 2021 •

edited

Loading

gabro commented Oct 14, 2021 •

edited

Loading

olafurpg commented Oct 14, 2021 •

edited

Loading

armanbilge commented Oct 14, 2021 •

edited

Loading